Skip to content

Remove support for legacy Parquet writer in Delta Lake#15436

Merged
ebyhr merged 3 commits intotrinodb:masterfrom
pajaks:pajaks/remove_legacy_writer
Jan 31, 2023
Merged

Remove support for legacy Parquet writer in Delta Lake#15436
ebyhr merged 3 commits intotrinodb:masterfrom
pajaks:pajaks/remove_legacy_writer

Conversation

@pajaks
Copy link
Copy Markdown
Member

@pajaks pajaks commented Dec 16, 2022 •
edited by ebyhr
Loading

Description

Remove support for legacy Parquet writer in Delta Lake connector

Additional context and related issues

Release notes

(x) Release notes are required, with the following suggested text:

# Delta Lake
* Remove support for non-optimzied Parquet writer.

@cla-bot
Copy link
Copy Markdown

cla-bot Bot commented Dec 16, 2022

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

ebyhr
ebyhr reviewed Dec 16, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update below places?

  • delta-lake.rst
  • BaseDeltaLakeMinioConnectorTest.testTargetMaxFileSize
  • io.trino.tests.product.deltalake.TestDeltaLakeDatabricksInsertCompatibility#testCompression - Please note that this test wasn't trigger in CI because PR came from forked repository

Comment thread .../src/test/java/io/trino/plugin/deltalake/TestDeltaLakeCreateTableStatisticsLegacyWriter.java Outdated
@cla-bot cla-bot Bot added the cla-signed label Dec 21, 2022
@pajaks pajaks marked this pull request as ready for review December 22, 2022 11:31
@pajaks pajaks marked this pull request as draft December 22, 2022 16:34
@ebyhr
Copy link
Copy Markdown
Member

ebyhr commented Jan 3, 2023 •
edited
Loading

Could you rebase on upstream to resolve conflicts?

@ebyhr ebyhr marked this pull request as ready for review January 3, 2023 23:44
ebyhr
ebyhr reviewed Jan 4, 2023
View reviewed changes
Comment thread plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeSessionProperties.java Outdated
Comment thread ...o-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeCreateTableStatistics.java Outdated
Comment thread ...o-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeCreateTableStatistics.java Outdated
@pajaks pajaks changed the title Remove support for legacy Parquet writer in Delta Lake DO NOT MERGE Remove support for legacy Parquet writer in Delta Lake Jan 4, 2023
@electrum
Copy link
Copy Markdown
Member

electrum commented Jan 4, 2023

We should wait for #9142 to be completed.

@pajaks pajaks requested review from findepi and homar January 5, 2023 14:08
@findepi
Copy link
Copy Markdown
Member

findepi commented Jan 5, 2023

We should wait for #9142 to be completed.

We currently use the new writer and it currently doesn't support LZ4.
Are you OK, @electrum , with users still using the old writer whenever they want to compress with LZ4?
I am concerned this means we keep supporting and (testing for) both writers.

Wdyt?

@electrum
Copy link
Copy Markdown
Member

electrum commented Jan 5, 2023

If the new writer is the default, then it's probably fine to merge this. We can implement LZ4 if anyone needs that.

@pajaks pajaks changed the title DO NOT MERGE Remove support for legacy Parquet writer in Delta Lake Remove support for legacy Parquet writer in Delta Lake Jan 9, 2023
ebyhr
ebyhr approved these changes Jan 11, 2023
View reviewed changes
Comment thread docs/src/main/sphinx/connector/delta-lake.rst Outdated
Comment thread plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakePageSink.java Outdated
@ebyhr
Copy link
Copy Markdown
Member

ebyhr commented Jan 27, 2023

Rebased on upstream to resolve conflicts on docs.

@pajaks
Copy link
Copy Markdown
Member Author

pajaks commented Jan 30, 2023

rebase on master to use CI fix #15879

@ebyhr
Copy link
Copy Markdown
Member

ebyhr commented Jan 30, 2023

/test-with-secrets sha=c1a989b99bd28382a8bdf89ab39ed44989c227db

@pajaks
Copy link
Copy Markdown
Member Author

pajaks commented Jan 31, 2023

suite-7-non-generic hit #14441

@ebyhr
Copy link
Copy Markdown
Member

ebyhr commented Jan 31, 2023

All tests passed in https://github.com/trinodb/trino/actions/runs/4049319103/jobs/6965546618. Let me merge now.

@ebyhr ebyhr merged commit dbc87c7 into trinodb:master Jan 31, 2023
@ebyhr ebyhr mentioned this pull request Jan 31, 2023
Closed
@colebow colebow added this to the 407 milestone Jan 31, 2023
@colebow colebow mentioned this pull request Jan 31, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ebyhr ebyhr ebyhr approved these changes

@findepi findepi Awaiting requested review from findepi

@homar homar Awaiting requested review from homar

Assignees

No one assigned

Labels

cla-signed docs

Milestone

407

Development

Successfully merging this pull request may close these issues.

5 participants

@pajaks @ebyhr @electrum @findepi @colebow